Skip to content

Conversation

devinivy
Copy link
Collaborator

@devinivy devinivy commented Feb 2, 2015

I made tons of updates here. It definitely resulted in breaking changes. But I think I've added many features and made the adapter function more naturally. If you'd rather not make such breaking changes now, that's understandable! Here's what I did:

  1. Allow for named global secondary indices using index: 'IndexName-hash' and index: 'IndexName-range'. Similarly, you specify primary keys as primaryKey: 'hash' and primaryKey: 'range'.

  2. Allow for named local secondary indices using index: 'secondary'. In that case, Vogels assumes that the index name is the name of the field, which the adapter respects.

  3. Cache Vogels models instead of recreating them all the time. They live in _vogelsReferences. To avoid confusion, I renamed _modelReferences to _collectionReferences.

  4. If a table name ends in an 's' then that 's' is removed before creating the Vogels model. That allows one to create a waterline collection with identity ending in 's' and use it normally. The waterline identity can then match the table name.

  5. I don't automatically create a primary key using keyId (since the name is created once explicitly for the table) or name indices using indexPrefix (because it seems like an unnecessary limitation).

  6. Removed the automatic creation of updatedAt and createdAt in the adapter. Standard waterline collection configurations should be able to deal with that.

  7. find automatically uses the most relevant index with the following precedence,

    Primary hash and range > primary hash and secondary range > global secondary hash and range
    > primary hash > global secondary hash > no index/primary
    
  8. update performs updates using conditional checks, so you can use conditions other than key conditions.

@gadelkareem
Copy link
Owner

Thank you for your effort.
I get an error while testing this code:

server-1 (err):         indexName = columnName;
server-1 (err):                     ^
server-1 (err): ReferenceError: columnName is not defined
server-1 (err):     at Object.module.exports.adapter._parseIndex (/var/www/server/node_modules/sails-dynamodb/index.js:298:21)

@devinivy
Copy link
Collaborator Author

devinivy commented Feb 2, 2015

Sorry, that was a silly oversight! It's a simple fix– I'll update soon.

@devinivy
Copy link
Collaborator Author

devinivy commented Feb 3, 2015

Should be resolved– let me know if it gives you any more issues. I expect it will not work with your current configuration, as

someName: {
  index: true
}

currently implies the key name is someName (the name of the column). It can be adjusted by using

someName: {
  index: 'someIndexName-hash'
}

using any index someIndexName.

@gadelkareem
Copy link
Owner

Can you add it to readme file so it would be documented?

@devinivy
Copy link
Collaborator Author

Do these additions seem adequate?

@gadelkareem
Copy link
Owner

I added a fix for endpoint while using a local dynamoDB and made changes to the model but I keep getting "Specified index is not part of table" for a model that does not have an index. Can you check?

@devinivy
Copy link
Collaborator Author

This occurs during a find?

@devinivy
Copy link
Collaborator Author

This bug should be resolved now.

@gadelkareem
Copy link
Owner

Creating a new record without specifying an id produces error: create error: { [Error: the value of id is not allowed to be undefined]

@devinivy
Copy link
Collaborator Author

I see that Vogels has an autogenerating UUID type– are you expecting to use that in a certain scenario? How should your primary key attribute be configured in order to use Vogels' UUID type? Some users may want their primary key hash to be something other than a UUID.

@gadelkareem
Copy link
Owner

Yes, It is part of sails framework activated by autoPK setting http://sailsjs.org/#/documentation/concepts/ORM/model-settings.html

@devinivy
Copy link
Collaborator Author

Okay, here's how I added autoPk support:

  1. I made the type for the autoPk default to "string"
  2. I made any auto-incrementing string attribute a UUID field, as UUIDs are the nearest way of supporting an auto-incrementing id in DynamoDB.

Because autoPk creates an auto-incrementing string attribute in the collection definition, it will become a UUID. How does that seem to you?

@gadelkareem
Copy link
Owner

Yes that is how it is currently working on master.

@devinivy
Copy link
Collaborator Author

Did that last commit make it work for you?

@gadelkareem
Copy link
Owner

Yes it does not seem to solve the problem
server (err): /var/www/server/node_modules/sails/node_modules/waterline/lib/waterline/error/WLError.js:36 server (err): this.rawStack = (this._e.stack.replace(/^Error(\r|\n)*(\r|\n)*/, '')); server (err): ^ server (err): TypeError: Cannot call method 'replace' of undefined server (err): at new WLError (/var/www/server/node_modules/sails/node_modules/waterline/lib/waterline/error/WLError.js:36:34)

@devinivy
Copy link
Collaborator Author

It is working for me. I'll take a closer look, though. We should try to write some tests.

I'm also confused, as that stack trace does not include any files in sails-dynamodb.

gadelkareem added a commit that referenced this pull request Mar 11, 2015
[Breaking Chgs] Indices, updates, model caching, etc.
@gadelkareem gadelkareem merged commit a104468 into gadelkareem:master Mar 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants